Skip to content

[MOD-17706] finalize SVSIndex::relabelVector - #1045

Open
nonirosenfeldredis wants to merge 16 commits into
mainfrom
sharon-svs-relabel
Open

[MOD-17706] finalize SVSIndex::relabelVector#1045
nonirosenfeldredis wants to merge 16 commits into
mainfrom
sharon-svs-relabel

Conversation

@nonirosenfeldredis

@nonirosenfeldredis nonirosenfeldredis commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Describe the changes in the pull request

SVS delegates label management to its library, so relabelVector was reported as unsupported. The library now offers replace_external_id, which renames an external id (single) or a label (multi) purely in the id translation table — leaving the dataset and graph untouched, which is the same guarantee the in-tree indexes give. This bumps the submodule to a7e3494 and routes relabelVector through it.

Preconditions are checked in svs.h rather than left to the library: it throws on a bad one, and this API answers with a code. Checking has_id for both labels first also means the throwing validation inside replace_external_id cannot fire, so no exception escapes into the C API. markIndexUpdate is deliberately not called — nothing was added or deleted and the label count is unchanged, so no consolidation is owed.

Availability gate. SVS_SHARED_LIB (Linux x86_64) downloads a pre-built SVS release rather than building the submodule, and replace_external_id is in none of v0.3.0 / v0.3.1 / v0.3.2 — nor is v0.3.2 an ancestor of SVS main. So availability depends on which SVS a build picked up, not on the platform. cmake/svs.cmake now detects it following the pattern already used for LVQ, with one necessary difference: LVQ tests for a header file's existence, but this is a method, so the check greps that header's contents. Where it is missing, the override is left out entirely and the interface default reports VecSimRelabel_Unsupported — telling a caller to fall back to delete + insert rather than read it as a no-op.

Python bindings. Two changes, both prerequisites for testing any of this from python:

  • relabel_vector had no binding, so no flow test could reach it. Bound on PyVecSimIndex (serves every index type), with VecSimRelabelCode exposed so callers can tell the rejections apart.
  • get_vector was bound, but its dispatch treated "not brute force" as "is HNSW". SVSIndex derives from VecSimIndexAbstract, not HNSWIndex, so the dynamic_cast returned nullptr and the virtual call dereferenced it — svs_index.get_vector(label) was a segfault, which is why the SVS getDataByLabel merged in Implement SVSIndex::getDataByLabel (MOD-17706) #1033 was unreachable from python. Since getDataByLabel is a pure virtual on VecSimIndexAbstract, the algo switch was never needed: call it virtually and BF, HNSW and SVS are all served. Tiered keeps its own branch because it wraps two VecSimIndexAbstract instances rather than being one.

Which issues this PR fixes

  1. MOD-17706

Main objects this PR modified

  1. SVSIndex::relabelVector — via impl_->replace_external_id, gated on availability
  2. cmake/svs.cmakeHAVE_SVS_REPLACE_EXTERNAL_ID detection
  3. PyVecSimIndexrelabel_vector binding, VecSimRelabelCode enum, getDataByLabel dispatch fix
  4. Unit + flow tests

Testing

Unit tests are gated the same way as the implementation, asserting real behavior where the API exists and the unsupported code where it does not, so the contract stays covered in both configurations. Flow tests cover both APIs, single and multi, across every index type.

Multi is not a formality: a label owns several vectors, each backend moves them by a different route, and a move onto an occupied label must be rejected because accepting it would silently merge two labels' vectors rather than lose one.

The tiered test relabels an early label and the last-inserted one. Workers ingest in insertion order, so the early one is already in HNSW while the late one is still buffered with pending ingest jobs — measured at ~690 of 1000 still buffered at relabel time — so both tiers are covered. The buffered case is the delicate one: a job left holding the old label would either ingest under it or throw out of a worker thread.

Both sides of the gate are verified, using real SVS checkouts rather than a forced macro:

Submodule cmake detection Build Relabel test present
a7e3494 (has API) replace_external_id found - SVS relabeling enabled, macro =1 clean relabelVector, relabelVectorRejects — pass
7786d43 (v0.3.0, no API) replace_external_id not found - reports unsupported, macro =0 clean, 0 errors relabelVectorUnsupported — passes

So the same source tree configures and builds correctly against either SVS, and a build without the API reports VecSimRelabel_Unsupported rather than failing to compile.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Follow-up, not blocking this PR

SVS relabeling stays Unsupported on Linux x86_64 until an SVS release carries intel/ScalableVectorSearch#383 and SVS_URL in cmake/svs.cmake is bumped to it. That is an external dependency; the gate makes the code correct and compiling in the meantime, which is what the table above verifies.

Notes for the reviewer

  • The tiered half of the get_vector dispatch fix is a pre-existing bug unrelated to SVS relabel — it fell out of doing the fix properly rather than special-casing SVS.
  • The new add_compile_definitions(VectorSimilarity PUBLIC ...) call copies the existing LVQ block verbatim for consistency. Worth knowing that form also defines bare VectorSimilarity and PUBLIC macros (visible in COMPILE_DEFS), since add_compile_definitions takes definitions rather than a target — pre-existing, and I did not change it.
  • Two unrelated local-environment snags, not addressed here: make flow_test cannot configure with CMake 4.x because pybind11 is pinned at v2.10.1, and poetry install needs setuptools in the venv on Python 3.13+.

🤖 Generated with Claude Code


Note

Medium Risk
Changes label identity and tiered locking around async ingest; incorrect relabel coordination could mis-key vectors in the buffer vs backend, though tests focus heavily on that race.

Overview
Adds in-place vector relabeling for SVS (and tiered SVS) when the linked SVS library exposes replace_external_id, with CMake feature detection (HAVE_SVS_REPLACE_EXTERNAL_ID) so pre-built SVS binaries without that API still compile and report VecSimRelabel_Unsupported.

SVS / tiered SVS: relabelVector validates labels and calls replace_external_id on the backend; the tiered path is all-or-nothing across buffer and backend, takes updateJobMutex plus std::scoped_lock on tier guards to avoid deadlocks with in-place add, and relabels the backend before the flat buffer.

Python: Exposes relabel_vector and VecSimRelabelCode; fixes get_vector to use VecSimIndexAbstract / tiered dispatch so SVS (and tiered) no longer crash on get_vector.

Tests: Flow and unit coverage for relabel on flat, HNSW, tiered HNSW, SVS, and tiered SVS (including multi-label and SVS skip when unsupported), plus SVS get_vector and tiered getDataByLabel edge cases.

Reviewed by Cursor Bugbot for commit 88be7eb. Bugbot is set up for automated code reviews on this repo. Configure here.

nonirosenfeldredis and others added 2 commits September 9, 2026 11:58
SVS delegates label management to its library, so relabeling was previously
reported as unsupported. The library now offers `replace_external_id`, which
renames an external id (single) or a label (multi) purely in the id translation
table, leaving the dataset and the graph alone -- exactly the guarantee the
in-tree indexes give. Bump the submodule to a7e3494, which adds it, and route
relabelVector through it.

Preconditions are checked here rather than left to the library: it throws on a
bad one, and this API answers with a code. Checking `has_id` for both labels
first also means the throwing validation inside `replace_external_id` cannot
fire, so no exception escapes into the C API. `markIndexUpdate` is deliberately
not called - nothing was added or deleted and the label count is unchanged, so
the index owes no consolidation.

Gated on availability, following the pattern svs.cmake already uses for LVQ.
This matters because SVS_SHARED_LIB (Linux x86_64) downloads a pre-built SVS
release rather than building the submodule, and none of v0.3.0/v0.3.1/v0.3.2
carries `replace_external_id`. LVQ tests for a header's existence; this is a
method, so the check greps that header instead. Where it is missing the
override is left out entirely and the interface default reports
`VecSimRelabel_Unsupported`, which tells a caller to fall back to delete +
insert rather than read it as a no-op.

The unit tests are gated the same way, asserting the real behavior where the
API exists and the unsupported code where it does not, so the contract stays
covered in both configurations.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two changes to the bindings, both needed before any of this could be tested
from python:

`relabel_vector` had no binding at all, so no flow test could reach it. Bind it
on PyVecSimIndex, which serves every index type, and expose VecSimRelabelCode so
a caller can tell the rejections apart rather than compare bare ints.

`get_vector` was already bound, but its dispatch treated "not brute force" as
"is HNSW". SVSIndex derives from VecSimIndexAbstract, not HNSWIndex, so the
dynamic_cast returned nullptr and the virtual call dereferenced it -
`svs_index.get_vector(label)` was a segfault, which is why the SVS
getDataByLabel merged in #1033 was unreachable from python. `getDataByLabel` is
a pure virtual on VecSimIndexAbstract, so the algo switch was never needed: call
it virtually and brute force, HNSW and SVS are all served. A tiered index wraps
two VecSimIndexAbstract instances rather than being one, so it keeps its own
branch - that fixes the same latent null dereference for tiered, which was
pre-existing and unrelated to SVS.

Flow tests cover both APIs, single and multi, across every index type. Multi is
not a formality here: a label owns several vectors and each backend moves them
by a different route, and a move onto an occupied label must be rejected because
accepting it would silently merge two labels' vectors rather than lose one.

The tiered test relabels an early label and the last-inserted one. Workers
ingest in insertion order, so the early one is already in HNSW while the late
one is still buffered with pending ingest jobs - measured at ~690 of 1000 still
buffered at relabel time - so both tiers get covered. The buffered case is the
delicate one: a job left holding the old label would either ingest under it or
throw out of a worker thread.

The SVS flow tests probe the build with a no-op relabel and skip when it reports
unsupported, since python cannot see HAVE_SVS_REPLACE_EXTERNAL_ID. The probe
uses equal labels, which is answered before `impl_` is touched, so it works on
an empty index and cannot mask a broken implementation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@nonirosenfeldredis
nonirosenfeldredis marked this pull request as ready for review September 9, 2026 09:15
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.44%. Comparing base (fd9fe8d) to head (88be7eb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
+ Coverage   97.42%   97.44%   +0.02%     
==========================================
  Files         141      141              
  Lines        8698     8855     +157     
==========================================
+ Hits         8474     8629     +155     
- Misses        224      226       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nonirosenfeldredis nonirosenfeldredis changed the title [MOD-17706] Implement SVSIndex::relabelVector [MOD-17706] finalize SVSIndex::relabelVector Sep 9, 2026
Comment thread src/VecSim/algorithms/svs/svs.h Outdated
}
// `impl_` is only created on the first insertion, so an index that never held a vector
// trivially has nothing under `old_label`.
if (!impl_ || !impl_->has_id(old_label)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAn you use the SVSIndex's isLabelExists()?

for old_label, new_label in moved.items():
assert index.relabel_vector(old_label, new_label) == VecSimRelabel_OK

index.wait_for_index()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also test get_vector while a vector is still buffered, and for a multi-value label whose vectors are split across both tiers?
The current tiered tests call it only after wait_for_index(), so they exercise retrieval after ingestion completes. Controlling the ingestion workers would make those additional states deterministic. The assertion should verify that every expected vector is returned exactly once.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested it in C++ now

void runGC() { VecSimTieredIndex_GC(index.get()); }

VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) {
return VecSimIndex_RelabelVector(index.get(), old_label, new_label);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need to find a way to synchronize the new relabel_vector binding with concurrent HNSW queries? knn_parallel releases the GIL and its workers hold indexGuard, but this call bypasses that guard. Relabeling writes idToMetaData[id].label while queries can read it through getExternalLabel, without a common lock.

Two review points from @dor-forer.

`SVSIndex::relabelVector` asked `impl_->has_id` directly, duplicating
`isLabelExists` twelve lines below it -- including its own null check on `impl_`,
which that method already covers.

The `relabel_vector` binding took no lock. Plain HNSW leaves synchronisation to
its caller: `relabelVector` takes `indexDataGuard` exclusively, but `topKQuery`
does not take it at all, so that guard orders relabel against other writers and
not against a reader resolving ids through `getExternalLabel`. `ElementMetaData`
is `#pragma pack(1)`, so the label store is unaligned and a racing reader can
observe a torn value rather than merely a stale one. `indexGuard` is the
binding's stand-in for the caller's lock and `knn_parallel` takes it shared, so
relabel now takes it exclusively, releasing the GIL first as everything else
that takes that guard does.

The same gap exists in the `add_vector` and `delete_vector` bindings, which are
also unguarded; left alone as pre-existing rather than widened into this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread src/python_bindings/bindings.cpp Outdated
nonirosenfeldredis and others added 4 commits September 10, 2026 21:21
Review asked for `get_vector` coverage while a vector is still buffered and for
a multi-value label split across the tiers. The split case already had
`getDataByLabelSpansBothTiers`, which steps the mock pool one job at a time, so
these fill what it leaves:

`getDataByLabelWhileStillBuffered` is the case that regressed before -- reading
only the backend reports nothing for a vector written recently enough to still
be queued, which is exactly when a document is most likely written again. It
never runs the job, so the state is deterministic rather than raced, and it
asserts the vector is reported once more after draining, from the backend.

`getDataByLabelInTheIngestWindow` pins what the two tiers holding one label at
once means, which differs by index kind and is a deliberate consequence of a
single condition in the dispatch: a single-value label short-circuits on the
buffer hit and never reads the backend, so it reports one vector; a multi-value
label always reads the backend, so a vector caught mid-ingest is reported twice.
The second half was documented on the declaration but nothing enforced it.

Both build their state directly instead of racing a worker into it.

Verified by mutation: dropping the buffer read makes the buffered test report
`Which is: 0`; dropping the single-value short-circuit makes the window test
report `Which is: 2`, and only that test, so each guards its own condition.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard added a worse failure than the one it removed, as @dor-forer spotted.
`PyHNSWLibIndex::createBatchIterator` takes `indexGuard` in shared mode and
holds it until the iterator is destroyed, releasing it from the iterator's
deleter. `std::shared_mutex` is neither recursive nor upgradeable, so

    it = index.create_batch_iterator(query)
    index.relabel_vector(7, 70)

blocks on a lock the calling thread already holds: undefined behaviour, in
practice a hang. That is reachable from ordinary single-threaded use, whereas
the race the lock addressed needs two Python threads -- `knn_parallel` joins its
workers before returning, so it can only overlap a relabel issued from another
thread.

So relabel goes back to being unguarded, which is also what `add_vector` and
`delete_vector` beside it do: this binding leaves serialisation to its caller.
The reason is now stated at the method, since "no lock here" is the kind of
thing that reads like an oversight.

Guarding it properly means not holding `indexGuard` across a batch iterator's
whole lifetime, which is pre-existing and its own change. A narrower option, if
we want one here: count live iterators on the index and have relabel raise a
Python error rather than block when one is alive.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`addVector`, `knn`, `range` and the batch iterator's `getNextResults` all
release the GIL around their index call; `deleteVector` and `relabel_vector` did
not. For relabel that is not just an inconsistency: on a tiered index it takes
`mainIndexGuard` exclusively, and a live batch iterator holds that guard shared
until it is depleted or freed, so relabel blocks there. Blocking while holding
the GIL stops the Python thread that owns the iterator from ever running to
release it, turning a wait into a cross-thread deadlock.

Releasing cannot self-deadlock, unlike taking `indexGuard` here.

It does not help the single-threaded case, where the same thread owns both the
iterator and the relabel and no amount of GIL handling saves it. That is a
property of tiered relabel taking the main guard exclusively where
`deleteLabelFromHNSW` takes it shared, and is tracked separately.

`deleteVector` has the same missing release; left alone as pre-existing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nonirosenfeldredis and others added 5 commits September 11, 2026 18:11
`SVSIndex::relabelVector` moves a label, but wrapping that index in a tier does
not inherit the ability: neither `TieredSVSIndex` nor `VecSimTieredIndex`
overrides `relabelVector`, so a tiered SVS index resolves to the interface
default and answers `Unsupported`. Only `TieredHNSWIndex` overrides it.

Nothing is wrong for a caller that honours the code -- `Unsupported` means fall
back to delete and re-add -- but it does mean a tiered SVS index never takes the
relabel path however capable its backend is, which is easy to miss when the
plain-index tests pass. `test_svs_tiered.cpp` had no relabel coverage at all,
so there was nothing stating either the behaviour or the gap.

Asserted with the label present and the target free, so the test pins the one
rejection that is about the index kind rather than the arguments, and so
implementing tiered SVS relabel has a test to flip rather than a silent change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`SVSIndex::relabelVector` moved a label, but wrapping that index in a tier did
not inherit the ability: neither `TieredSVSIndex` nor `VecSimTieredIndex`
overrode `relabelVector`, so a tiered SVS index fell through to the interface
default and answered `Unsupported`. That is correct for a caller which honours
the code, but it meant a tiered SVS index never took the relabel path however
capable its backend was -- and tiered is how it is used.

Both tiers are asked, because a multi-value label routinely has copies in each
and a target taken in either would collide once the buffer drains. The backend
moves first, since it is the tier that can refuse -- `Unsupported` when built
against an SVS without `replace_external_id` -- and refusing after the buffer
had moved would leave the label half applied.

`updateJobMutex` is taken first, in the order `updateSVSIndex` takes its own
locks. That is not defensive: an update job snapshots the buffer's labels *by
value* and afterwards reconciles only id swaps and deletions, so a rename
landing between the snapshot and the backend insert is invisible to it and the
vector reaches the backend under the old label, leaving it under both.

All three guards are held across the checks and the mutations. Checking under
shared locks and reacquiring exclusively would be cheaper for rejections, but
`std::shared_mutex` cannot upgrade, and async `addVector` and `deleteVector`
need neither `updateJobMutex` nor a held guard -- so either could land in the
gap and leave the move applied to one tier only.

Tests mirror what insertion has for this mode. `MovesTheLabelInBothWriteStates`
covers buffered-with-a-pending-job and moved-to-the-backend, the states
`addVector` and `insertJob` cover; `RejectsOnATier` covers each code, with the
taken target tried in both tiers; `DuringUpdateJob` is the `insertJobAsync`
analogue, relabelling 200 labels against live workers. The fixture's type set
spans single, multi and Quant_8, so this also answers whether a compressed
backend can move a label: it can, since `replace_external_id` renames an id and
never touches the stored vector.

`CannotLandInsideAnUpdateJobsWindow` is what pins the mutex, via the
`UpdateJob::before_add_to_svs` tracing hook that sits exactly in the window.
Worth stating why it exists: `DuringUpdateJob` still passes with the mutex
removed, so concurrency alone does not demonstrate the need. The hook test
fails, reporting a label count of 2 for one vector. It relabels from another
thread, since the job holds the mutex throughout and relabelling inline would
block on a lock the thread already holds.

Not added: relabel concurrent with a query. The tiers share
`VecSimTieredIndex::topKQueryImp`, so SVS inherits the cross-tier duplicate that
#1047 fixes, and such a test belongs with that fix rather than failing here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`test_svs_tiered.py` had eleven insertion tests and no relabel coverage, which
is how the tier answering `Unsupported` went unnoticed. Now that it moves a
label, exercise it through the binding as well, following the helper-plus-thin-
wrappers shape the insertion tests use, across unquantized, 8-bit, LeanVec 8x8
and FLOAT16.

Thresholds are small enough that ingestion is under way when the relabels run,
so the moved labels land in both states -- one early label already in the
backend, one late label still buffered -- and the assertions after
`wait_for_index` are what fails if an in-flight update job ingested a vector
under the label it snapshotted rather than the one it now has.

`get_vector` is only asserted for the unquantized configuration: a compressed
backend reports no stored values, so there the move is checked by searching for
the vector and expecting the new label, which holds either way.

Multi is not covered because a multi-label tiered SVS index is unsupported and
`IndexCtx` asserts against it.

Not run locally: `make pybind` fails before compiling, on CMake 4.4.1 against
pybind11's `cmake_minimum_required(<3.5)`. CI is the first execution of these.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`relabelVector` and `relabelVectorRejects` were plain `TEST(SVSTest, ...)`, not
`TYPED_TEST`. The suite name matched the typed fixture, which made them look
parameterized, but they built one index inline with no `quantBits` and ran
twice rather than six times -- so `replace_external_id` under quantization was
never exercised on a plain index, only through a tier.

Converting them needed the distance assertions rethought. They demanded exactly
0 for a vector's own query, which only holds unquantized. Capturing the distance
before the move and requiring it unchanged afterwards says what the test is
actually for -- a rename must not disturb the stored vector -- without assuming
what the distance is. It holds exactly in all three modes, so the move really is
bit-identical for the data.

`test_svs_multi.cpp` had no relabel coverage at all, though BF-multi and
HNSW-multi both do. A multi-value label is where a move that handles only the
label's first id still passes every single-value test, so each copy is checked
by its own distance, and an untouched neighbour label is kept alongside so a
move that is too broad shows up too.

Verified by mutation: returning OK without calling `replace_external_id` fails
`relabelVector` in all three modes and `relabelVectorMulti` with "copy 0 was
disturbed by the move", so the assertions are load-bearing rather than passing
because nothing moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`relabelVectorMarkedDeleted` existed only for plain HNSW. SVS reaches the same
contract by different means, so it is worth stating there too: SVS deletes
softly -- the entry is marked and only dropped by a later consolidation, so it
is still occupying an id -- but `has_id` excludes it, which is what makes the
move report the label absent rather than renaming a tombstone. The test also
adds the label back afterwards, to show the refusal left it free for reuse
rather than half-claimed.

The tier gets its own, because the delete can land while the vector is still
buffered or after it reached the backend and the label must be reported absent
either way, in whichever tier the delete missed. One thing that shaped it: no
tombstone is observable there -- `getNumMarkedDeleted()` reads 0 after the
delete, unlike on a plain index -- so it asserts the refusal and the absence
rather than the marking, and is named for what it actually covers.

Mutation notes, since the two tests differ in strength. Accepting a
marked-deleted label in `SVSIndex::relabelVector` fails the plain test in all
three modes. It does not fail the tiered one: `TieredSVSIndex::relabelVector`
checks `isLabelExists` itself before delegating, so both layers refuse
independently. The tiered test guards the contract across the two write states
rather than either single check.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread tests/unit/test_svs_tiered.cpp
nonirosenfeldredis and others added 3 commits September 11, 2026 22:28
The relabel counterpart to `test_parallel_insert_search`: the operation on one
thread, queries on another, with the relabels aimed at labels still being
ingested so a label is briefly held by both tiers.

The two tests differ in what they can assert, and that is the point. Inserting
concurrently makes a query legitimately miss vectors, so the insert test can
only check that recall did not regress. A relabel adds and removes nothing, so
the invariant is exact: a reply must never list one label twice. That is the
defect -- `merge_result_lists` collapses a vector both tiers report by matching
labels, and a relabel inside a query's window moves that key, so the two copies
survive as one vector under two labels.

Skipped on this branch. Not because the assertion is unsound -- a duplicate
label in a reply is always wrong -- but because the fix is on another branch
(#1047, MOD-18494) and the failure is intermittent, so leaving it live would
redden this PR's CI for a defect it did not introduce. Remove the marker once
that lands; nothing else about the test changes.

Being a canary is the honest description: the window is a few instructions, so
it can pass on a broken build. It cannot report a failure that is not real,
which is the acceptable direction. The deterministic coverage is the hook-based
C++ tests on #1047.

`import pytest` is added because `common.py` does not import it and the skip
marker needs it -- the same omission that bit `test_svs.py` earlier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A test that never runs earns nothing. It was added skipped because the fix it
depends on is on another branch, which makes it dead weight here: it cannot
catch a regression, and the marker is one more thing to notice and remove later.
The deterministic coverage for this is the hook-based C++ tests on #1047, which
is also where a flow-level version belongs once that lands.

`import pytest` goes with it; nothing else in the file used it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI caught this on Linux x86_64, where the build downloads a pre-built SVS
without `replace_external_id`: six failures, `relabelVectorMovesTheLabelInBoth
WriteStates` and `relabelVectorCannotLandInsideAnUpdateJobsWindow` across all
three type variants, the first reporting `Which is: 4` -- Unsupported -- where
it expected OK. `test_svs_tiered.cpp` had no occurrence of the macro at all,
unlike `test_svs.cpp` and `test_svs_multi.cpp`.

Gating the tests is only half of it, and the CI run shows why. The other three
tiered tests *passed* on that build, including `relabelVectorDuringUpdateJob`,
which asserts OK for two hundred moves. They passed because a rejection is
decided before the backend is consulted, and because a buffered label is
renamed in the flat buffer without asking the backend at all. So on Linux
x86_64 today, relabel on a tiered SVS index succeeds for a buffered label and
refuses an ingested one -- a capability that depends on which tier happens to
hold the label, which no caller can act on.

So `TieredSVSIndex::relabelVector` is gated too, mirroring
`SVSIndex::relabelVector`. Without the override the interface default reports
`Unsupported` for the whole tier, uniformly.

That also repairs the python probe. `svs_relabel_supported` asks
`relabel_vector(label, label)` and reads anything other than `Unsupported` as
capable, which is sound only while the method is absent; the tiered override
answered `SameLabel` before consulting the backend, so it would have looked
capable on a build that is not. The probe moves to `common.py` and the tiered
flow helper now uses it -- those tests were ungated in the same way, which the
review comment implies but does not spell out.

Verified both directions against real submodule checkouts. With
`replace_external_id` present: 15/15 tiered relabel runs pass. Without it
(submodule 7786d43, macro 0): compiles clean, and the only relabel test left in
the binary is `SVSTest.relabelVectorUnsupported` -- the `#else` branch that is
meant to be there -- which passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 98e7edc. Configure here.

Comment thread src/VecSim/algorithms/svs/svs_tiered.h Outdated
The orders in this class disagree. Nearly every path takes `flatIndexGuard`
before `mainIndexGuard`, but in-place `addVector` takes main *before* flat while
the backend is still empty:

    std::shared_lock backend_shared_lock(this->mainIndexGuard);
    if (this->backendIndex->indexSize() == 0) {
        std::lock_guard lock(this->flatIndexGuard);

Relabel acquired its three guards in sequence, flat before main, which closes a
cycle with that path: relabel holds flat and waits for main, the add holds main
and waits for flat.

Taking all three through one `std::scoped_lock` fixes it. `std::lock` try-locks
with back-off, so relabel never holds one guard while blocking on another and
cannot participate in a cycle whichever order the other side uses. It is also
the idiom that same path already uses for its own two locks.

To be precise about the report that prompted this: the `scoped_lock(updateJobMutex,
mainIndexGuard)` branch is not a second cycle. `std::lock` releases and retries
rather than holding one while blocking on the other, so that branch cannot
deadlock against relabel. The empty-backend branch above is the real one.

The test pins it, and a regression surfaces as the global 300s timeout rather
than an assertion, since a deadlock hangs. Verified by mutation: restoring the
sequential acquisition leaves the test running with nothing completed after 45
seconds, against 1ms with the scoped lock.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants